✅ Deploy Preview for endearing-brigadeiros-63f9d0 canceled.

Codecov Report

❌ Patch coverage is 83.72093% with 98 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.77%. Comparing base (ddff723) to head (256ef3f).

@@            Coverage Diff             @@
##             main    #1043      +/-   ##
==========================================
- Coverage   83.25%   82.77%   -0.49%     
==========================================
  Files          59       66       +7     
  Lines        2449     2787     +338     
  Branches      280      335      +55     
==========================================
+ Hits         2039     2307     +268     
- Misses        366      433      +67     
- Partials       44       47       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Picked up a couple of test failures after merging main - will resolve (and start working on the additional tests needed).

I can release the unique constraint on the index to avoid this - however, I put it in to catch invalid data as, like the use of the repository name as an ID, it will result in the selection of the wrong repo project at times (although in this case the multiple records would have to be the same repo) and I thought it better for tests to fail etc. if data wasn't cleaned up from a previous run. Where do you think we should go with this @jescalada - an automated migration seems difficult as you'd just have to delete or programmatically edit one of the duplicate records...

I think catching and displaying a simple error message with the invalid entry/entries could be enough - so that the GitProxy administrator can quickly identify the issue and fix it manually. Thankfully, the error seems to occur on backend (db) startup, so end users wouldn't really have the app suddenly blowing up.

@andypols in the earlier iterations of this PR I was using the proxy middlewares directly in the Router and the only good way to add/remove them again was to restart. I ended up changing that to wrap them in a custom middleware which uses the proxy middlewares in the proxies object. That approach can more easily support update without a restart, but it'd take a refactor of the proxy router to support updating (probably wrapping it in a class, or just merge it into the Proxy class) and a new function on the Proxy class to trigger the update (to replace the stop/start calls made in the api.

I'm loathe to take that on before we merge the current state, as I think restarting for a new origin is a low (but not no!) impact flaw and we could deal with it in a subsequent issue/PR.

@kriswest

I'm loathe to take that on before we merge the current state, as I think restarting for a new origin is a low (but not no!) impact flaw and we could deal with it in a subsequent issue/PR.

Fair enough — I can see that. It was just a question, not a criticism. From my (possibly overly simplistic) perspective, the proxy shouldn't care what the Git domain is. Happy to see it handled in a subsequent PR!

@andypols

It was just a question, not a criticism

No problem! You've just got me thinking (out loud).

From my (possibly overly simplistic) perspective, the proxy shouldn't care what the Git domain is.

One thing that lead to it is the express-http-proxy middleware taking the proxied domain as an argument - it will accept a function, but those can only rotate which host requests are proxied to as the function doesn't have the request itself in scope). I guess another approach would be to create the proxy middleware in response to each request; I didn't do that on the (untested!) assumption that it would could cause a performance bottleneck (e.g. by checking the proxied hosts in the DB on each request). However, that's likely a small penalty...

@kriswest Thank you very much for your help. Although the issue hasn't been resolved yet, I've taken a look at some of the suggestions you provided.

To debug I would be tempted to log the request in chain.ts, before and after parsing by parseAction and see what you get. ..

I added a console.log() to parseAction, but the request doesn't have a body (no req.body !! log printed). I'm not sure what part of the log I should pay close attention to. I'm struggling to debug due to my limited knowledge and skills.

diff --git a/src/proxy/chain.ts b/src/proxy/chain.ts
index 2130519c..94446568 100644
--- a/src/proxy/chain.ts
+++ b/src/proxy/chain.ts
@@ -32,6 +32,9 @@ let pluginsInserted = false;
 export const executeChain = async (req: any, res: any): Promise<Action> => {
   let action: Action = {} as Action;
   try {
+    console.log(req);
+    if (!req.body)
+        console.log("no req.body !!");
     action = await proc.pre.parseAction(req);
     const actionFns = await getChain(action);

@@ -54,6 +57,7 @@ export const executeChain = async (req: any, res: any): Promise<Action> => {
     }
   }

+  console.log(action);
   return action;
 };

• 19.2 against a github repository with the same client and similar git config - is the issue specific to this PR?

Unfortunately, our company's firewall policy blocks push to github.com. I'll try testing from home later.

• a different gitlab project clone, ensuring that there are changes to push - is the issue specific to the repo/checkout?

This issue also occurs in other projects and repositories.

P.S. There is a second PR that you may need #973 - relating to linking PRs to authors via email. I doubt that is involved in the issue, ..

I tried to merge PR 973 on this PR, but a conflict occurred. I'll likely need to try again after this PR and 973 are merged.

Hi @kyet, hopefully 973 will be through review and merged shortly. I'll update this shortly after.

However, I can't think of anything (in the PR) that would result in in req.body being undefined (I'm assuming its undefined rather than null), but there are reasons that it can be including the client not sending a body (or something in between you and the server removing it). Are you testing on localhost, if so we can discount something on your network.

Happy to look at the request and headers in case it tells us something.

Conflicts resolved and ready for a another look. I haven't had a chance to test it yet, but the tests are all passing.

I'm aware I haven't done anything in the documentation regarding this PR. That should probably be reviewed and work to add to the docs undertaken under a new issue.

Looks good! Just a few comments on the failing tests due to pre-existing GitLab origin (as well as other origins). It'd be fantastic if we could fix those to be agnostic of the data in the database.

Making the tests agnostic of the data would be good, but I haven't figured out how yet - the requests that go out are affected the response from the other end, so we'd probably need to mock something in the proxy, perhaps the URL it would forward a request on to, and then check it had been called and returned the right value?

@kriswest I've taken another look at the tests, and since the failing ones are "end to end" if anything, it'd be harder than I thought to mock out the database dependency (and it only really makes sense for unit/function tests).

As long as the reason for failure is obvious for contributors, that should be enough for now so we can speed up the release. We can make the tests more robust in another issue (#978 and #1143 are related to this).

Hi, @kriswest. I found culprit of the No body found problem!

I dumped the packets, I found that my git client was actually sending the contents.

The pattern matching fails in the code below, and therefore the req.body is not generated.

proxy/routes/index.ts

const isPackPost = (req: Request) =>
  req.method === 'POST' &&
  // eslint-disable-next-line no-useless-escape
  /^\/[^\/]+\/[^\/]+\.git\/(?:git-upload-pack|git-receive-pack)$/.test(req.url);

const teeAndValidate = async (req: Request, res: Response, next: NextFunction) => {
  if (!isPackPost(req)) return next();
  ..
	try {
	  ..
		(req as any).body = buf;

In my case, req.url is /git.mygitlab.com/my.name/git-proxy.git/git-receive-pack (sanitized real name).

Upon reviewing the regex, I noticed that there is a change history in the commit below.

29e7d2a

You may need to revert or modify the regex changes.

I would like to share one more thing related to this issue. As I mentioned earlier, our company has a firewall issue that prevents us from pushing to github.com, so I tested it at home. The test environment is different, but when I set up a github.com proxy like this, proxy fails, but proxy2 succeeds.

[remote "proxy"]
        url = http://localhost:8000/github.com/kyet/git-proxy.git
        fetch = +refs/heads/*:refs/remotes/proxy/*
[remote "proxy2"]
        url = http://localhost:8000/kyet/git-proxy.git
        fetch = +refs/heads/*:refs/remotes/proxy2/*

If I think about it in relation to the above issue, when falling back to the default proxy, I assume that req.url probably doesn't contain github.com (e.g., /kyet/git-proxy.git/git-receive-pack)